Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issuer metadata to indicate PoP requirement. #87

Merged
merged 4 commits into from
Dec 14, 2023
Merged

Conversation

cobward
Copy link
Contributor

@cobward cobward commented Oct 19, 2023

Addresses #89

Currently a wallet has no way of knowing whether the issuing authority requires a proof of possession in the Credential Request. From my point of view there are two ways we can remedy this:

  1. Add a parameter in the credentials_supported issuer metadata object that indicates whether a PoP is required.
  2. Add some normative text which would require wallets to send a PoP if proof_types_supported in the credentials_supported issuer metadata object is not omitted and contains at least one element.

Personally I prefer explicit indication over implied so I have raised this PR showing option 1, but I am open to other opinions and options.

@paulbastian
Copy link
Contributor

There is also the option, that the Wallet may know this implicit, depending on whether the Issuer sends a c_nonce in the Token Response. But I think putting it in the Issuer metadata is the right direction, as there may also be more requirements on the keys and other requirements such as wallet attestations that must be reflected in the metadata

@@ -1177,6 +1177,7 @@ This section defines the structure of the objects that appear in the `credential
* `cryptographic_binding_methods_supported`: OPTIONAL. Array of case sensitive strings that identify how the Credential is bound to the identifier of the End-User who possesses the Credential as defined in (#credential-binding). Support for keys in JWK format [@!RFC7517] is indicated by the value `jwk`. Support for keys expressed as a COSE Key object [@!RFC8152] (for example, used in [@!ISO.18013-5]) is indicated by the value `cose_key`. When Cryptographic Binding Method is a DID, valid values MUST be a `did:` prefix followed by a method-name using a syntax as defined in Section 3.1 of [@!DID-Core], but without a `:`and method-specific-id. For example, support for the DID method with a method-name "example" would be represented by `did:example`. Support for all DID methods listed in Section 13 of [@DID_Specification_Registries] is indicated by sending a DID without any method-name.
* `cryptographic_suites_supported`: OPTIONAL. Array of case sensitive strings that identify the cryptographic suites that are supported for the `cryptographic_binding_methods_supported`. Cryptographic algorithms for Credentials in `jwt_vc` format should use algorithm names defined in [IANA JOSE Algorithms Registry](https://www.iana.org/assignments/jose/jose.xhtml#web-signature-encryption-algorithms). Cryptographic algorithms for Credentials in `ldp_vc` format should use signature suites names defined in [Linked Data Cryptographic Suite Registry](https://w3c-ccg.github.io/ld-cryptosuite-registry/).
* `proof_types_supported`: OPTIONAL. A JSON array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports. Supported values include those defined in (#proof_types) or other values defined in a profile of this specification or elsewhere. If omitted, the default value is `jwt`. `proof_type` claim is defined in (#credential_request).
* `proof_of_possession_required`: OPTIONAL. A JSON boolean that indicates whether a proof of possession must be submitted when requesting a credential of this format. If this field is omitted it can assumed to be `false`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `proof_of_possession_required`: OPTIONAL. A JSON boolean that indicates whether a proof of possession must be submitted when requesting a credential of this format. If this field is omitted it can assumed to be `false`.
* `proof_of_possession_required`: OPTIONAL. Boolean value specifying whether Credential Issuer requires a proof of possession to be submitted in the Credential Request, with `true` indicating support. If omitted, the default value is `true`.

aligning with the rest of the language. also, why is default false..?

Copy link
Collaborator

@tlodderstedt tlodderstedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Kristina to fold the newly proposed metadata claim into the existing one and change its name

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed in a WG call. agreed that there is a need to clarify when PoP is needed but with an improvement. Specific suggestion is as following:
there are currently two parameters: proof_types_supported and proof_of_possession_required. the presence of proof_types_supported alone can be used to indicate that PoP is required because for the same credential type, PoP is either required or not (no use-case when same type is sometimes issued without PoP and other times with PoP), but _supported has a different nuance from _required so suggetion is to rename proof_types_supported to proof_types and clarify that when it is omitted, it means "PoP not required".

Comment on lines 1196 to 1197
* `proof_types_supported`: OPTIONAL. A JSON array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports. Supported values include those defined in (#proof_types) or other values defined in a profile of this specification or elsewhere. If omitted, the default value is `jwt`. `proof_type` claim is defined in (#credential_request).
* `proof_of_possession_required`: OPTIONAL. Boolean value specifying whether Credential Issuer requires a proof of possession to be submitted in the Credential Request, with `true` indicating support. If this field is omitted it can assumed to be `false`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge into one parameter proof_types, where if omitted, the default value is PoP not required.

Suggested change
* `proof_types_supported`: OPTIONAL. A JSON array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports. Supported values include those defined in (#proof_types) or other values defined in a profile of this specification or elsewhere. If omitted, the default value is `jwt`. `proof_type` claim is defined in (#credential_request).
* `proof_of_possession_required`: OPTIONAL. Boolean value specifying whether Credential Issuer requires a proof of possession to be submitted in the Credential Request, with `true` indicating support. If this field is omitted it can assumed to be `false`.
* `proof_types`: REQUIRED when Credential Issuer requires a proof of possession of the cryptographic key material to be submitted in the Credential Request. Array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports as defined in (#credential_request). Supported values include those defined in (#proof_types) or other values defined in a profile of this specification or elsewhere. If omitted, Credential Issuer does not require proof of possession of the cryptographic key material.

something like this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sakurann your suggestions looks good to me.

@@ -710,7 +710,7 @@ For cryptographic binding, the Client has the following options to provide crypt
A Client makes a Credential Request to the Credential Endpoint by sending the following parameters in the entity-body of an HTTP POST request using the `application/json` media type.

* `format`: REQUIRED when the `credential_identifier` was not returned from the Token Response. MUST NOT be used otherwise. String that determines the format of the Credential to be issued, which may determine the type and any other information related to the Credential to be issued. Credential Format Profiles consisting of the Credential format specific set of parameters are defined in (#format_profiles). When this parameter is used, `credential_identifier` parameter MUST NOT be present.
* `proof`: OPTIONAL. Object containing the proof of possession of the cryptographic key material the issued Credential would be bound to. The `proof` object MUST contain a following claim:
* `proof`: OPTIONAL. Object containing the proof of possession of the cryptographic key material the issued Credential would be bound to. The `proof` object is REQUIRED if the `proof_of_possession_required` parameter in the issuer metadata for the submitted credential format is `true`. The `proof` object MUST contain a following claim:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `proof`: OPTIONAL. Object containing the proof of possession of the cryptographic key material the issued Credential would be bound to. The `proof` object is REQUIRED if the `proof_of_possession_required` parameter in the issuer metadata for the submitted credential format is `true`. The `proof` object MUST contain a following claim:
* `proof`: OPTIONAL. Object containing the proof of possession of the cryptographic key material the issued Credential would be bound to. The `proof` object is REQUIRED if the `proof_types` parameter is present in the issuer metadata. The `proof` object MUST contain a following claim:

aligning with below

@cobward
Copy link
Contributor Author

cobward commented Dec 4, 2023

@Sakurann I've made some changes based on your suggestions, but I tried to streamline the language a little.

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would like to be more explicit when parameter is omitted

@@ -1196,7 +1196,7 @@ This specification defines the following Credential Issuer Metadata:
* `scope`: OPTIONAL. A JSON string identifying the scope value that this Credential Issuer supports for this particular Credential. The value can be the same accross multiple `credentials_supported` objects. The Authorization Server MUST be able to uniquely identify the Credential Issuer based on the scope value. The Wallet can use this value in the Authorization Request as defined in (#credential-request-using-type-specific-scope). Scope values in this Credential Issuer metadata MAY duplicate those in the `scopes_supported` parameter of the Authorization Server.
* `cryptographic_binding_methods_supported`: OPTIONAL. Array of case sensitive strings that identify how the Credential is bound to the identifier of the End-User who possesses the Credential as defined in (#credential-binding). Support for keys in JWK format [@!RFC7517] is indicated by the value `jwk`. Support for keys expressed as a COSE Key object [@!RFC8152] (for example, used in [@!ISO.18013-5]) is indicated by the value `cose_key`. When Cryptographic Binding Method is a DID, valid values MUST be a `did:` prefix followed by a method-name using a syntax as defined in Section 3.1 of [@!DID-Core], but without a `:`and method-specific-id. For example, support for the DID method with a method-name "example" would be represented by `did:example`. Support for all DID methods listed in Section 13 of [@DID_Specification_Registries] is indicated by sending a DID without any method-name.
* `cryptographic_suites_supported`: OPTIONAL. Array of case sensitive strings that identify the cryptographic suites that are supported for the `cryptographic_binding_methods_supported`. Cryptographic algorithms for Credentials in `jwt_vc` format should use algorithm names defined in [IANA JOSE Algorithms Registry](https://www.iana.org/assignments/jose/jose.xhtml#web-signature-encryption-algorithms). Cryptographic algorithms for Credentials in `ldp_vc` format should use signature suites names defined in [Linked Data Cryptographic Suite Registry](https://w3c-ccg.github.io/ld-cryptosuite-registry/).
* `proof_types_supported`: OPTIONAL. A JSON array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports. Supported values include those defined in (#proof_types) or other values defined in a profile of this specification or elsewhere. If omitted, the default value is `jwt`. `proof_type` claim is defined in (#credential_request).
* `proof_types`: OPTIONAL. Array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports as defined in (#credential_request). If this array is non-empty and present, the Credential Issuer requires proof of possession of the cryptographic key material.
Copy link
Collaborator

@Sakurann Sakurann Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `proof_types`: OPTIONAL. Array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports as defined in (#credential_request). If this array is non-empty and present, the Credential Issuer requires proof of possession of the cryptographic key material.
* `proof_types`: OPTIONAL. Array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports as defined in (#credential_request), one of which is MUST be used in the Credential Request. If this array is non-empty and present, the Credential Issuer requires proof of possession of the cryptographic key material. If the parameter is omitted or array is empty, Credential Issuer does not require proof of possession of the cryptographic key material.

No need to be explicit what to do when PoP is not present..?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "supports" needs to be changed to "requires" as this is the semantics of the array.

"If this array is non-empty and present, the Credential Issuer requires proof of possession of the cryptographic key material."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed in the Dec-7-2023 WG. agreed to merge once this suggestion is accepted.

@Sakurann Sakurann added this to the ID-1 milestone Dec 7, 2023
@@ -1196,7 +1196,7 @@ This specification defines the following Credential Issuer Metadata:
* `scope`: OPTIONAL. A JSON string identifying the scope value that this Credential Issuer supports for this particular Credential. The value can be the same accross multiple `credentials_supported` objects. The Authorization Server MUST be able to uniquely identify the Credential Issuer based on the scope value. The Wallet can use this value in the Authorization Request as defined in (#credential-request-using-type-specific-scope). Scope values in this Credential Issuer metadata MAY duplicate those in the `scopes_supported` parameter of the Authorization Server.
* `cryptographic_binding_methods_supported`: OPTIONAL. Array of case sensitive strings that identify how the Credential is bound to the identifier of the End-User who possesses the Credential as defined in (#credential-binding). Support for keys in JWK format [@!RFC7517] is indicated by the value `jwk`. Support for keys expressed as a COSE Key object [@!RFC8152] (for example, used in [@!ISO.18013-5]) is indicated by the value `cose_key`. When Cryptographic Binding Method is a DID, valid values MUST be a `did:` prefix followed by a method-name using a syntax as defined in Section 3.1 of [@!DID-Core], but without a `:`and method-specific-id. For example, support for the DID method with a method-name "example" would be represented by `did:example`. Support for all DID methods listed in Section 13 of [@DID_Specification_Registries] is indicated by sending a DID without any method-name.
* `cryptographic_suites_supported`: OPTIONAL. Array of case sensitive strings that identify the cryptographic suites that are supported for the `cryptographic_binding_methods_supported`. Cryptographic algorithms for Credentials in `jwt_vc` format should use algorithm names defined in [IANA JOSE Algorithms Registry](https://www.iana.org/assignments/jose/jose.xhtml#web-signature-encryption-algorithms). Cryptographic algorithms for Credentials in `ldp_vc` format should use signature suites names defined in [Linked Data Cryptographic Suite Registry](https://w3c-ccg.github.io/ld-cryptosuite-registry/).
* `proof_types_supported`: OPTIONAL. A JSON array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports. Supported values include those defined in (#proof_types) or other values defined in a profile of this specification or elsewhere. If omitted, the default value is `jwt`. `proof_type` claim is defined in (#credential_request).
* `proof_types`: OPTIONAL. Array of case sensitive strings, each representing `proof_type` that the Credential Issuer supports as defined in (#credential_request). If this array is non-empty and present, the Credential Issuer requires proof of possession of the cryptographic key material.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "supports" needs to be changed to "requires" as this is the semantics of the array.

"If this array is non-empty and present, the Credential Issuer requires proof of possession of the cryptographic key material."

@cobward cobward requested a review from tlodderstedt December 12, 2023 18:11
@cobward
Copy link
Contributor Author

cobward commented Dec 12, 2023

This is how the text currently reads:

Array of case sensitive strings, each representing a proof_type that the Credential Issuer supports as defined in (#credential_request), one of which MUST be used in the Credential Request. If this array is non-empty and present, the Credential Issuer requires proof of possession of the cryptographic key material. If the parameter is omitted or array is empty, Credential Issuer does not require proof of possession of the cryptographic key material.

I didn't quite understand your comment Torsten. Do you want the highlighted word to be changed from supports to requires?

@Sakurann
Copy link
Collaborator

Sakurann commented Dec 13, 2023

If you accepted my suggestion, @tlodderstedt agreed during the WG call that the language suggested addressed his concerns.

I think a sentence quoted in Torsten's comment is not the one he meant - and he was looking to clarify if the wallet need to support all or at least one of the proof types supported by the issuer.

so this one should be good to go - thank you, Jacob!

@cobward
Copy link
Contributor Author

cobward commented Dec 13, 2023

If you accepted my suggestion, @tlodderstedt agreed during the WG call that the language suggested addressed his concerns.

I think a sentence quoted in Torsten's comment is not the one he meant - and he was looking to clarify if the wallet need to support all or at least one of the proof types supported by the issuer.

so this one should be good to go - thank you, Jacob!

Great, thanks for clearing that up Kristina

Copy link
Contributor

@tplooker tplooker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with one minor editorial nit

Co-authored-by: Tobias Looker <[email protected]>
@@ -713,7 +713,7 @@ For cryptographic binding, the Client has the following options to provide crypt
A Client makes a Credential Request to the Credential Endpoint by sending the following parameters in the entity-body of an HTTP POST request using the `application/json` media type.

* `format`: REQUIRED when the `credential_identifier` was not returned from the Token Response. MUST NOT be used otherwise. String that determines the format of the Credential to be issued, which may determine the type and any other information related to the Credential to be issued. Credential Format Profiles consisting of the Credential format specific set of parameters are defined in (#format_profiles). When this parameter is used, `credential_identifier` parameter MUST NOT be present.
* `proof`: OPTIONAL. Object containing the proof of possession of the cryptographic key material the issued Credential would be bound to. The `proof` object MUST contain a following claim:
* `proof`: OPTIONAL. Object containing the proof of possession of the cryptographic key material the issued Credential would be bound to. The `proof` object is REQUIRED if the `proof_types` parameter is non-empty and present in the `credentials_supported` map of the issuer metadata for the requested credential. The `proof` object MUST contain a following claim:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `proof`: OPTIONAL. Object containing the proof of possession of the cryptographic key material the issued Credential would be bound to. The `proof` object is REQUIRED if the `proof_types` parameter is non-empty and present in the `credentials_supported` map of the issuer metadata for the requested credential. The `proof` object MUST contain a following claim:
* `proof`: OPTIONAL. Object containing the proof of possession of the cryptographic key material the issued Credential would be bound to. The `proof` object is REQUIRED if the `proof_types` parameter is present in the `credentials_supported` map of the issuer metadata for the requested credential and non-empty. The `proof` object MUST contain a following claim:

@Sakurann Sakurann merged commit 32c146c into openid:main Dec 14, 2023
2 checks passed
@cobward cobward deleted the patch-1 branch December 14, 2023 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants